Skip to content

feat(sfs): make resource pool wait timeouts configurable - #1740

Merged
marceljk merged 4 commits into
stackitcloud:mainfrom
devpie:fix/sfs-resource-pool-timeouts
Sep 14, 2026
Merged

marceljk merged 4 commits into
stackitcloud:mainfrom
devpie:fix/sfs-resource-pool-timeouts

Conversation

@devpie

@devpie devpie commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Description

relates to #1737

stackit_sfs_resource_pool cannot create a pool that STACKIT needs more than 10 minutes to
provision. CreateResourcePoolWaitHandler and its update/delete counterparts set
SetTimeout(10 * time.Minute), and the resource passed a context without a deadline, so that
default was the only limit and no configuration could reach it.

This adds a timeouts attribute (create/read/update/delete) following the pattern of
dremio/instance.

Why a context deadline and not SetTimeout

core/wait.WaitWithContext applies the handler's own timeout only when the incoming context
carries no deadline
:

if _, ok := ctx.Deadline(); !ok {
	ctx, cancel = context.WithTimeout(ctx, h.timeout)
	defer cancel()
}

(core v0.26.0, wait.go)

Setting a context deadline per CRUD method therefore replaces the hardcoded value, and no
SetTimeout call is needed. Defaults stay at the wait handler value plus
core.DefaultTimeoutMargin, so unconfigured resources keep their current behaviour.
TestSfsResourcePoolCreateTimeout checks that a configured timeouts.create bounds the create.

The wait error

When a wait ends because this context's deadline expired, the error names the configured timeout and
suggests raising it. The wait handler reports terminal error states and failing polls through the
same error, so the hint is added only on the deadline path, and identically in create, update and
delete. The hint is utils.TimeoutHint in stackit/internal/utils, so other resources can reuse it;
TestTimeoutHint pins its text.

On point 2 of the issue

The issue also asks to keep the resource in state when the wait times out. That is already
implemented: utils.SetAndLogStateFields writes the IDs before the wait (CONTRIBUTING.md states
this as project doctrine, and TestSfsResourcePoolSavesIDsOnError covers it), the framework
initialises the create response state to a null object so no unknowns leak, and Terraform keeps the
object and marks it tainted rather than discarding it. What follows is a replace, not a lost entry.
No provider change was needed for that, only the corrected wording of the error.

testdata/resource-pool-max.tf sets timeouts, and no ImportStateVerifyIgnore is needed for it: terraform-plugin-testing deletes timeouts and timeouts.* from
both sides of the comparison unconditionally, after the ignore loop
(testing_new_import_state.go:398-411, v1.16.0).
The DNS acceptance tests rely on the same behaviour.

Not changed

sfs/share has the same hardcoded 10 minutes in all three of its wait handlers. Out of scope for
this issue — happy to follow up if wanted.

Checklist

  • Issue was linked above
  • Code format was applied: make fmt
  • Examples were added / adjusted (see examples/ directory) — deliberately not: no example in examples/ has ever carried a timeouts block (git log -S timeouts -- examples/ is empty), and feat(dns) add timeouts to dns resources and datasources #1345, which added timeouts to the DNS resources, put the demonstrable configuration in testdata/resource-max.tf instead. Hardcoding durations on a registry page would also pin numbers that go stale when the SDK waiter default moves.
  • Docs are up-to-date: make generate-docs (will be checked by CI)
  • Unit tests got implemented or updated
  • Acceptance tests got implemented or updated — testdata/resource-pool-max.tf now sets timeouts, mirroring dns/testdata/resource-max.tf, so TestAccResourcePoolResourceMax covers the attribute across create, import-verify and update
  • Unit tests are passing: make test (will be checked by CI)
  • No linter issues: make lint (will be checked by CI)

@marceljk marceljk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! I left some minor comments

Comment thread stackit/internal/services/sfs/resourcepool/resource.go Outdated
Comment thread stackit/internal/services/sfs/resourcepool/resource.go Outdated
Comment thread stackit/internal/services/sfs/resourcepool/resource.go
Comment thread stackit/internal/services/sfs/resourcepool/resource.go
Comment thread docs/resources/sfs_resource_pool.md
Comment thread stackit/internal/services/sfs/sfs_test.go Outdated
@devpie

devpie commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up in 7f8825e, after re-reading the result of the previous commit:

  • the update wait branch still carried the summary Error creating resource pool while the appended hint named timeouts.update, so the diagnostic contradicted itself. Summary and detail now say "updating", matching what fix(sfs): guard against nil responses from the API #1741 changes the same line to, so the two should not conflict.
  • the hint starts on its own line again; concatenating it onto the wrapped error ran the sentences together.
  • the data source read timeout got the same blank line you asked for in the resource, and the acceptance-test data source now declares a timeouts block, as dns/testdata/resource-max.tf does for its data source.

I also removed a paragraph from the PR description that still described the state write you asked me to drop.

Comment thread stackit/internal/services/sfs/resourcepool/resource.go Outdated
CreateResourcePoolWaitHandler and its update/delete counterparts default to
10 minutes. The resource passed a context without a deadline, so that default
was the only limit and no configuration could reach it. A pool that STACKIT
needs longer than 10 minutes to provision could not be created at all.

The SDK wait handler applies its own timeout only when the incoming context
carries no deadline (core/wait.WaitWithContext). Setting a context deadline in
each CRUD method therefore replaces the hardcoded value, which is what the new
`timeouts` attribute does. Defaults stay at the wait handler value plus
core.DefaultTimeoutMargin, so unconfigured resources keep their behavior.

The configured timeouts are written to state together with the IDs before the
create wait starts. Without that, a failed wait leaves an entry whose refresh
and destroy fall back to the default timeouts - on exactly the recovery path
those values are needed for.

The error raised when the create wait handler gives up now says that Terraform
marks the resource tainted and replaces it on the next run, names `untaint` and
the import ID, and mentions `timeouts.create` only when this context's deadline
is what ended the wait. The handler reports terminal error states and failing
polls through the same error, which are not timeouts.

TestWaitHandlerTimeoutIsBoundedByContext pins the SDK behavior the attribute
depends on, so an SDK bump that enforces the handler timeout unconditionally
fails the build instead of silently capping the configured value again.
- shorten the create wait error to the timeout hint, and emit the same hint in
  update and delete so the three read consistently
- drop the write of the timeouts attribute into the partial state
- add the timeouts attribute to the resource pool data source as well
- replace the create timeout test with the shorter form used for dns, on the
  existing MockServer
- keep a blank line before the timeout blocks in Read and Update
…e review

The update wait branch kept the summary "Error creating resource pool" while
the appended hint named `timeouts.update`, so the diagnostic contradicted
itself. Summary and detail now say "updating", matching what stackitcloud#1741 changes the
same line to.

The hint starts on its own line again; concatenating it directly onto the
wrapped error ran the two sentences together.

Two follow-ups in the spirit of the review rather than its letter: the data
source read timeout gets the same blank line that was asked for in the
resource, and the acceptance-test data source declares a timeouts block, as the
dns testdata does for its data source.
The hint that names the configured timeout after a wait ran out is not
specific to SFS, so it now lives in stackit/internal/utils as
TimeoutHint where other resources and data sources can use it. The
move adds a unit test that pins the exact text, including the leading
newline, and checks that a cancellation or a wait that failed before
the deadline produce no hint.
@marceljk
marceljk force-pushed the fix/sfs-resource-pool-timeouts branch from 06224ce to 825422b Compare September 14, 2026 08:21
@marceljk
marceljk merged commit 32d7806 into stackitcloud:main Sep 14, 2026
3 checks passed
devpie added a commit to devpie/terraform-provider-stackit that referenced this pull request Sep 14, 2026
stackitcloud#1740 landed the configurable wait timeouts on the same lines this branch
guards. The update wait now reports the merged form: summary and detail from
main, including the timeout hint, followed by the nil check from this branch.
sfs_test.go keeps the tests of both sides.
devpie added a commit to devpie/terraform-provider-stackit that referenced this pull request Sep 14, 2026
Three diagnostics inside Update paths were copied from Create and still
reported a creation failure. stackitcloud#1740 fixed the resource pool update wait; this
covers the ID guard next to it and both update diagnostics of the share
resource. Only the summaries change, the details are accurate as they are.
@devpie
devpie deleted the fix/sfs-resource-pool-timeouts branch September 14, 2026 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants